Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

i/5692: Added documentation for the config.toolbar.shouldNotGroupWhenFull option #201

Merged
merged 2 commits into from
Nov 20, 2019

Conversation

oleq
Copy link
Member

@oleq oleq commented Nov 18, 2019

@coveralls
Copy link

coveralls commented Nov 18, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling ee2b5ee on i/5692 into a1a0f33 on master.

@@ -162,6 +164,11 @@
* * **`toolbar.viewportTopOffset`** – The offset (in pixels) from the top of the viewport used when positioning a sticky toolbar.
* Useful when a page with which the editor is being integrated has some other sticky or fixed elements
* (e.g. the top menu). Thanks to setting the toolbar offset the toolbar will not be positioned underneath or above the page's UI.
* * **`toolbar.shouldGroupWhenFull`** – When `true` (default), the toolbar will group its items that
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation entry is growing (items, viewportTopOffset and now shouldGroupWhenFull). I know it's convenient for developers to keep it in a single place (module:core/editor/editorconfig~EditorConfig#toolbar) but if we get more options like this, I think it's time to define some @interface ToolbarConfig and put the detailed documentation there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as those options are fairly simple (boolean, number, simple array), I'd keep them locally here because, as you mentioned, that more convenient to read. But I agree that at some point we may need to extract it. But then we can keep a lot of information here locally, and have details in the interface.

* viewportTopOffset: 30
* viewportTopOffset: 30,
*
* shouldGroupWhenFull: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* shouldGroupWhenFull: true
* shouldGroupWhenFull: false

No one will be setting it to true as this is the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd rather rename it to shouldNotGroupWhenFull to avoid a flag which is default by true. Then you'll also have less issues with handling the default in the other two PRs.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented.

@oleq oleq changed the title i/5692: Added documentation for the config.toolbar.shouldGroupWhenFull option i/5692: Added documentation for the config.toolbar.shouldNotGroupWhenFull option Nov 19, 2019
@Reinmar Reinmar merged commit 3af9e3a into master Nov 20, 2019
@Reinmar Reinmar deleted the i/5692 branch November 20, 2019 11:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add possibility to toggle toolbar items grouping in a build
3 participants